Allow setting an HRN in invoice_requests built by `pay_for_offer` #3903

TheBlueMatt · 2025-07-01T14:33:52Z

If a user did their own BIP 353 lookup to fetch an offer from a
human readable name, we still want them to be able to use
`ChannelManager::pay_for_offer`.

Because BIP 353 offer payments require that the `invoice_request`
include the human readable name, we need to add an argument to set
the `invoice_request` HRN to `pay_for_offer`, which we do here.

and a few other misc cleanups of the pay_for_offer API.

ldk-reviews-bot · 2025-07-01T14:33:54Z

👋 I see @tankyleo was un-assigned.
If you'd like another reviewer assignemnt, please click here.

`ChannelManager::pay_for_offer` and `ChannelManager::pay_for_offer_from_human_readable_name` have accumulated a handful of arguments, most of which we expect users to never actually set. Instead, here, we move `quantity` and `payer_note` (which we expect users to never set) as well as `route_params_config` and `retry_strategy` (which we expect users to generally stick with the defaults on) into a new struct, which implements `Default`. This cleans up a good bit of cruft on payment calls.

tnull

While I see the motivation for these changes, I'm not super convinced about some of the API changes in this PR.

tnull · 2025-07-02T06:38:29Z

lightning/src/ln/channelmanager.rs

+	///
+	#[cfg_attr(
+		feature = "dnssec",
+		doc = "Note that setting this will cause [`ChannelManager::pay_for_offer_from_human_readable_name`] to fail."


This is confusing. Why move everything to this struct if we introduce a footgun for the user?

More generally, do we really care that much that the LDK method has three more arguments or so?

Its not a huge deal, no, but it did seem like an opportunity to hide parameters we expect to never actually be used behind a default(). Especially the quantity just feels like a weird thing to put in the top-level function signature, though also the routing parameters.

I'm open to dropping the move here (or, honestly, dropping the quantity argument entirely and just setting it to 1 if the offer needs it), if you feel strongly.

(or, honestly, dropping the quantity argument entirely and just setting it to 1 if the offer needs it), if you feel strongly.

Hmm, not sure if we can just drop it, but it does have some footguns that would be nice to tackle at some point, see for example #3233

But presumably @jkczyz would have a stronger/better informed view on how to fix this/how the API should look like.

Seems pay_for_offer_from_human_readable_name should take a different type and have it translate it to OptionalOfferPaymentInfo. Or alternatively just leave quantity as a parameter to pay_for_offer.

Or we could introduce an OfferSupportingQuantity wrapper type on Offer -- which can only be created when Offer::expects_quantity is true -- and have a separate pay_for_offer_using_quantity method taking that type and having a required quantity parameter. Then drop the quantity parameter from pay_for_offer -- setting it internally to 1 if Offer::expects_quantity is true.

Or we could introduce an OfferSupportingQuantity wrapper type on Offer -- which can only be created when Offer::expects_quantity is true -- and have a separate pay_for_offer_using_quantity method taking that type and having a required quantity parameter. Then drop the quantity parameter from pay_for_offer -- setting it internally to 1 if Offer::expects_quantity is true.

Not sure why we need a separate wrapper type? I went ahead and pushed a fixup that adds a separate pay_for_offer_with_quantity type.

Okay, that's fine by me.

Wrapper would just remove a possible error where otherwise an offer not expecting a quantity is passed to pay_for_offer_with_quantity.

That would just move the error somewhere else, though? Instead of getting an error back from pay_for_offer_with_quantity you get a Result when you convert to the wrapper, which is just more code for the user.

lightning/src/ln/channelmanager.rs

tnull · 2025-07-02T06:44:28Z

lightning/src/ln/channelmanager.rs

@@ -11215,7 +11218,7 @@ where
 	/// [Avoiding Duplicate Payments]: #avoiding-duplicate-payments
 	pub fn pay_for_offer(
 		&self, offer: &Offer, amount_msats: Option<u64>, payment_id: PaymentId,
-		optional_info: OptionalOfferPaymentInfo,
+		optional_info: OptionalOfferPaymentInfo, derived_from_hrn: Option<HumanReadableName>,


If we do that, why not actually lean into the switch to bitcoin-payment-instructions and drop the pay_for_offer_from_hrn method. I don't think any of our users really implements HRNs yet, so we wouldn't even break API for anybody, AFAIK.

Yea, we could. Its still useful for a lightning-only wallet, but maybe we don't care too much about those?

What determines if something goes in OptionalOfferPaymentInfo vs added as a new parameter. Seems derived_from_hrn would rarely be Some, so why add another parameter for it?

My thinking was basically "anything that we expect users to need to think about" ie either "we dont have a good default and the user knows best" or "if the user mis-configures this, that's really bad". derived_from_hrn isn't expected to be set that much, but it falls into the second category.

Hmm, I find the parameter a bit odd, especially now that we try to split out most to a params object.

Yea, we could. Its still useful for a lightning-only wallet, but maybe we don't care too much about those?

Do we believe there ever will be a truly Lightning-only wallet that doesn't support to pay any onchain (or other L2) addresses?

IMO, even if we think this will ever be a thing, we should probably not optimize our API around that. Now that we added pay_for_offer_with_quantity, could we add a similar pay_for_offer_derived_from_hrn method that allows setting the HumanReadableName param, and just drop the original pay_for_offer_from_human_readable_name?

Hmm, I find the parameter a bit odd, especially now that we try to split out most to a params object.

Sure, but in this case it falls into the "a default becomes dangerous if it actually needed to be configured" case, so its kinda hard to move...

Do we believe there ever will be a truly Lightning-only wallet that doesn't support to pay any onchain (or other L2) addresses?

Maybe not anymore, I dunno. Phoenix was for a long time pre-splicing, I could see an eventual future where you don't really need L1 payments, and all L2s just "accept lightning" so it doesn't matter. I'm okay with removing the pay_for_offer_from_hrn stuff if you feel strongly that we should just rip it out.

IMO, even if we think this will ever be a thing, we should probably not optimize our API around that.

Hmm? This is the opposite case - the new API is important for wallets that do support L1 payments, and thus need to use bitcoin-payment-instructions to do resolutions.

Now that we added pay_for_offer_with_quantity, could we add a similar pay_for_offer_derived_from_hrn method that allows setting the HumanReadableName param, and just drop the original pay_for_offer_from_human_readable_name?

My fear is that a wallet will use bitcoin-payment-instructions to resolve an HRN, then happily pass the offer to pay_for_offer, since that's "the offer payment method". In practice, most users won't ever read through our long docs, so payments to BOLT 12 HRNs will just fail (while LNURL requests will succeed so the wallet author won't notice until later while BOLT 12/353 HRNs aren't broadly deployed yet).

I suppose we could try to address this by having bitcoin-payment-instructions instead have a pay_with_ldk_channelmanager method that takes the PaymentId and OptionalOfferPaymentParams and passes them through to a separate method, but it still leaves the API quite easy to misuse.

Could we have bitcoin-payment-instructions return an OfferFromHRN (wrapping the offer and name) that we could pass to our own pay_for_offer_derived_from_hrn ? Then we don't need to pass None everywhere pay_for_offer is called.

Hmm, it does still seem easy to accidentally call channelmanager.pay_for_offer(offer_from_hrn.offer()...), though certainly less so than exposing the offer directly. I'm a bit torn on it but willing to go that direction if y'all prefer.

lightning/src/ln/channelmanager.rs

jkczyz · 2025-07-02T13:51:25Z

lightning/src/ln/channelmanager.rs

@@ -11215,7 +11218,7 @@ where
 	/// [Avoiding Duplicate Payments]: #avoiding-duplicate-payments
 	pub fn pay_for_offer(
 		&self, offer: &Offer, amount_msats: Option<u64>, payment_id: PaymentId,
-		optional_info: OptionalOfferPaymentInfo,
+		optional_info: OptionalOfferPaymentInfo, derived_from_hrn: Option<HumanReadableName>,


What determines if something goes in OptionalOfferPaymentInfo vs added as a new parameter. Seems derived_from_hrn would rarely be Some, so why add another parameter for it?

lightning/src/ln/channelmanager.rs

jkczyz · 2025-07-02T14:32:22Z

lightning/src/ln/channelmanager.rs

+	///
+	#[cfg_attr(
+		feature = "dnssec",
+		doc = "Note that setting this will cause [`ChannelManager::pay_for_offer_from_human_readable_name`] to fail."


Seems pay_for_offer_from_human_readable_name should take a different type and have it translate it to OptionalOfferPaymentInfo. Or alternatively just leave quantity as a parameter to pay_for_offer.

Or we could introduce an OfferSupportingQuantity wrapper type on Offer -- which can only be created when Offer::expects_quantity is true -- and have a separate pay_for_offer_using_quantity method taking that type and having a required quantity parameter. Then drop the quantity parameter from pay_for_offer -- setting it internally to 1 if Offer::expects_quantity is true.

For some time we've automatically opened a connection to the blinded path introduction point when we need to send a message we generated. However, the "Limitations" section in `ChannelManager::pay_for_offer` lists having a direct connection as required for the payment to succeed, which is not true. Instead, we simply remove the section from both `pay_for_offer` and `pay_for_offer_from_human_readable_name`.

If a user did their own BIP 353 lookup to fetch an offer from a human readable name, we still want them to be able to use `ChannelManager::pay_for_offer`. Because BIP 353 offer payments require that the `invoice_request` include the human readable name, we need to add an argument to set the `invoice_request` HRN to `pay_for_offer`, which we do here.

tnull

Fixups look good to me. Some more comments on the API design

tnull · 2025-07-08T08:33:35Z

lightning/src/ln/channelmanager.rs

+	/// pay the actual [`Bolt12Invoice`] once it is received.
+	///
+	/// This method is identical to [`Self::pay_for_offer`] with the one exception that it allows
+	/// you to specify the [`InvoiceRequest::quantity`]. We expect this to be rather seldom used,


nit: used seldomly?

tnull · 2025-07-08T08:36:55Z

lightning/src/ln/channelmanager.rs

+	///
+	#[cfg_attr(
+		feature = "dnssec",
+		doc = "Note that setting this will cause [`ChannelManager::pay_for_offer_from_human_readable_name`] to fail."


Okay, that's fine by me.

lightning/src/ln/channelmanager.rs

tnull · 2025-07-08T08:40:51Z

lightning/src/ln/channelmanager.rs

@@ -11215,7 +11218,7 @@ where
 	/// [Avoiding Duplicate Payments]: #avoiding-duplicate-payments
 	pub fn pay_for_offer(
 		&self, offer: &Offer, amount_msats: Option<u64>, payment_id: PaymentId,
-		optional_info: OptionalOfferPaymentInfo,
+		optional_info: OptionalOfferPaymentInfo, derived_from_hrn: Option<HumanReadableName>,


Hmm, I find the parameter a bit odd, especially now that we try to split out most to a params object.

Yea, we could. Its still useful for a lightning-only wallet, but maybe we don't care too much about those?

Do we believe there ever will be a truly Lightning-only wallet that doesn't support to pay any onchain (or other L2) addresses?

IMO, even if we think this will ever be a thing, we should probably not optimize our API around that. Now that we added pay_for_offer_with_quantity, could we add a similar pay_for_offer_derived_from_hrn method that allows setting the HumanReadableName param, and just drop the original pay_for_offer_from_human_readable_name?

ldk-reviews-bot requested a review from tankyleo July 1, 2025 14:44

TheBlueMatt force-pushed the 2025-07-bpi branch from bf47f4b to befb3f2 Compare July 1, 2025 18:59

TheBlueMatt force-pushed the 2025-07-bpi branch from befb3f2 to 076b2b3 Compare July 1, 2025 23:32

tnull reviewed Jul 2, 2025

View reviewed changes

tnull requested review from jkczyz and removed request for tankyleo July 2, 2025 06:47

jkczyz reviewed Jul 2, 2025

View reviewed changes

f switch to a dedicated pay_for_offer_with_quantity method

61308c4

TheBlueMatt force-pushed the 2025-07-bpi branch from 076b2b3 to 7a7a5d1 Compare July 7, 2025 14:09

TheBlueMatt added 7 commits July 7, 2025 14:26

f s/OptionalOfferPaymentInfo/OptionalOfferPaymentParams/g

37b04b1

f sp

4d5da56

f rebase derived_from_hrn

b97b58f

Mention the bitcoin-payment-instructions crate in docs

03cb4a1

f slightly better docs

a2daa01

TheBlueMatt force-pushed the 2025-07-bpi branch from 7a7a5d1 to a2daa01 Compare July 7, 2025 14:29

tnull reviewed Jul 8, 2025

View reviewed changes

Allow setting an HRN in invoice_requests built by pay_for_offer #3903

Are you sure you want to change the base?

Allow setting an HRN in invoice_requests built by pay_for_offer #3903

Conversation

TheBlueMatt commented Jul 1, 2025

Uh oh!

ldk-reviews-bot commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Allow setting an HRN in invoice_requests built by `pay_for_offer` #3903

Allow setting an HRN in invoice_requests built by `pay_for_offer` #3903

ldk-reviews-bot commented Jul 1, 2025 •

edited

Loading

tnull left a comment •

edited

Loading